-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Increase sleep in Risco setup #77619
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (untested)
Move sleep to library
f8554de
to
e460f47
Compare
OK, after some more investigation, I was able to confirm (don't ask me how :) ) that Risco is recommending waiting 5 seconds upon disconnection. Through testing, I was also able to see that when connecting too quickly, the panel continues to communicate encrypted, even before the encryption key was obtained by the client, which is what's causing the problem. Given that, I figured that it's probably best to move the sleep to the library: This also fixes reloading for the config entry (e.g. when changing options). The downside is that (I think) it can delay shutdown, but I'm not sure. |
We don't unload config entries on shutdown last time I checked so it shouldn't be an issue If you need to disconnect on shutdown you need to listen for the stop event. You could add that and call a different function in the lib that avoids the sleep in a future PR if it turns out to be needed |
@@ -154,6 +154,10 @@ async def async_unload_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: | |||
"""Unload a config entry.""" | |||
unload_ok = await hass.config_entries.async_unload_platforms(entry, PLATFORMS) | |||
if unload_ok: | |||
if is_local(entry): | |||
local_data: LocalData = hass.data[DOMAIN][entry.entry_id] | |||
await local_data.system.disconnect() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this ever happen in the tests? If so it needs to be patched out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked test runtime before committing, so it looks like disconnect()
is patched whenever this is used. It does make sense to add a specific test to make sure it's actually called, though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
RE HA shutdown - it's ok that we're not calling |
I think that is a rare case, but maybe it won't be in the future |
Breaking change
Proposed change
Risco setup is stuck when trying to connect too quickly after closing the first connection (the first connection is in the config flow, the second one in
async_setup_entry
. The initial sleep of 1 second seemed to have solved it, but upon further testing 5 seconds seems like a safer value.Type of change
Additional information
Checklist
black --fast homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: